Fixup #2196 #2221
Fixup #2196 #2221
Conversation
joschmitt
added
bugfix
|
Yay, this is breaking now. It requires signatures |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2221 +/- ##
==========================================
- Coverage 88.00% 87.99% -0.01%
==========================================
Files 127 127
Lines 31803 31807 +4
==========================================
+ Hits 27989 27990 +1
- Misses 3814 3817 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I would vote for the temporary fallback, but don't have a strong opinion. |
Use `@inferred` correctly and fix return type
joschmitt
force-pushed
the
js/inferred
branch
from
2409e84 to
0a94404
Compare
joschmitt
force-pushed
the
js/inferred
branch
from
0a94404 to
a445ea3
Compare
joschmitt
force-pushed
the
js/inferred
branch
from
a445ea3 to
6a4051d
Compare
|
I added a fallback, so this should not break anything anymore. Please review and merge :) |
| function exponent_vector(::Type{Vector{S}}, a::MPoly{T}, i::Int) where {T <: RingElement, S} | ||
| e = Vector{S}(undef, nvars(parent(a))) |
There was a problem hiding this comment.
This seems a bit unusual. I'd expect this to be either
| function exponent_vector(::Type{Vector{S}}, a::MPoly{T}, i::Int) where {T <: RingElement, S} | |
| e = Vector{S}(undef, nvars(parent(a))) | |
| function exponent_vector(::Type{S}, a::MPoly{T}, i::Int) where {T <: RingElement, S} | |
| e = S(undef, nvars(parent(a))) |
(one could also restrict S to e.g. S <: Vector or S <: AbstractVetor)
Or else
| function exponent_vector(::Type{Vector{S}}, a::MPoly{T}, i::Int) where {T <: RingElement, S} | |
| e = Vector{S}(undef, nvars(parent(a))) | |
| function exponent_vector(::Type{S}, a::MPoly{T}, i::Int) where {T <: RingElement, S} | |
| e = Vector{S}(undef, nvars(parent(a))) |
(and then change the callers to pass in just Int resp. eltype(someVectorType)
There was a problem hiding this comment.
I intentionally did not implement the second option because there is/was this idea floating around that one should also be able to get the exponent vectors as ZZMatrix. Then I would find it inconsistent if there are exponent_vector(ZZRingElem, ...), which gives a Vector, and exponent_vector(ZZMatrix, ...).
I can implement the first variant. I might have preferred my version because it is easier to get the integer type that way, but I don't remember.
There was a problem hiding this comment.
Now I'm wondering, is there a functional difference between ::Type{Vector{S}} where S and ::Type{S} where S <: Vector? As in, does one type get accepted by one but not the other?
There was a problem hiding this comment.
Not the same.
julia> f(::Type{Vector{S}}) where {S} = S
f (generic function with 1 method)
julia> g(::Type{S}) where {S <: Vector} = S
g (generic function with 1 method)
julia> f(Vector)
ERROR: MethodError: no method matching f(::Type{Vector})
There was a problem hiding this comment.
Makes sense. That sounds to me like an argument not to change anything here?
3 participants
I realized that I used
@inferredwrong in the tests for #2196. Because of this, the tests didn't notice thatexponent_vectors(Vector{UInt}, f)did not returnVector{UInt}s.